transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

eshitachandwani · 2024-11-29T16:55:03Z

Fixes: #7556

Remove environment variable detection during transport creation. Introduce a per-address attribute to specify the proxy CONNECT string. This change aligns with the xDS design and provides more control over proxy behavior.
Create a new resolver that handles proxy configuration and delegates to child resolvers for target and proxy address resolution.
Introduce a new DialOption to preserve the current behavior of grpc.Dial, where the resolved target address is used in the proxy CONNECT request. This option ensures backward compatibility for users relying on the existing behavior.

RELEASE NOTES:

Target resolution is no longer performed on the client when using default resolvers with grpc.NewClient in conjunction with proxy environment variables.
New WithTargetResolutionEnabled() dial option added to explicitly force target URI resolution on the client.
When non-default resolvers are used alongside proxy environment variables, target resolution is now performed on the client.

codecov · 2024-11-29T16:59:56Z

Codecov Report

Attention: Patch coverage is 86.55462% with 16 lines in your changes missing coverage. Please review.

Project coverage is 82.24%. Comparing base (724f450) to head (69037b0).
Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/testutils/proxyserver/proxyserver.go	78.37%	12 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7881      +/-   ##
==========================================
- Coverage   82.28%   82.24%   -0.05%     
==========================================
  Files         381      385       +4     
  Lines       38539    38825     +286     
==========================================
+ Hits        31712    31931     +219     
- Misses       5535     5583      +48     
- Partials     1292     1311      +19

Files with missing lines	Coverage Δ
clientconn.go	`92.45% <100.00%> (-0.33%)`	⬇️
dialoptions.go	`90.80% <100.00%> (+1.82%)`	⬆️
internal/proxyattributes/proxyattributes.go	`100.00% <ø> (ø)`
.../resolver/delegatingresolver/delegatingresolver.go	`77.77% <100.00%> (+8.91%)`	⬆️
internal/transport/http2_client.go	`91.47% <100.00%> (-0.23%)`	⬇️
internal/transport/proxy.go	`77.35% <100.00%> (+9.35%)`	⬆️
internal/transport/transport.go	`91.56% <ø> (+6.82%)`	⬆️
resolver_wrapper.go	`84.16% <100.00%> (+1.28%)`	⬆️
internal/testutils/proxyserver/proxyserver.go	`78.37% <78.37%> (ø)`

... and 31 files with indirect coverage changes

purnesh42H · 2024-12-03T04:42:29Z

internal/testutils/proxy.go

+}
+
+// NewProxyServer create and starts a proxy server.
+func NewProxyServer(lis net.Listener, requestCheck func(*http.Request) error, errCh chan error, doneCh chan struct{}, backendAddr string, resolutionOnClient bool, proxyServerStarted func()) *ProxyServer {


may be we can do something like this fake server in its own package

grpc-go/internal/testutils/xds/fakeserver/server.go

Line 19 in 17d08f7

// Package fakeserver provides a fake implementation of the management server.

? wdyt?

New should only create the object and StartServer/Run should start the go routine to accept requests

purnesh42H · 2024-12-03T04:43:58Z

internal/transport/http2_client.go

 	address := addr.Addr
+
+	//if the ProxyConnectAddr is set in the aattribute, do a proxy dial.


typo: attribute

internal/transport/proxy.go

purnesh42H · 2024-12-03T04:46:30Z

internal/transport/proxy.go

-func proxyDial(ctx context.Context, addr string, grpcUA string) (net.Conn, error) {
-	newAddr := addr
-	proxyURL, err := mapAddress(addr)
+// proxyDial dials, connecting to a proxy first if necessary. Dials, does the


we can remove the necessary part now? It will always dial proxy if called?

test/proxy_test.go

internal/proxyattributes/proxyattributes_test.go

purnesh42H · 2024-12-05T09:06:26Z

internal/proxyattributes/proxyattributes_test.go

+		Addr:       "test-address",
+		Attributes: attributes.New(userAndConnectAddrKey, attr{user: nil, addr: "proxy-address"}),
+	}
+


nix new line

test/proxy_test.go

purnesh42H · 2024-12-05T09:54:59Z

test/proxy_test.go

+
+// Tests grpc.NewClient with the default "dns" resolver and dial option
+// enabling target resolution on the client and verifies that the resolution
+// happens on client.


nit: mention the dial option WithTargetResolutionEnabled()

test/proxy_test.go

purnesh42H · 2024-12-05T09:59:14Z

test/proxy_test.go

+
+// Tests grpc.NewClient with grpc.WithNoProxy() set and verifies that it does
+// not dail to proxy, but directly to backend.
+func (s) TestGrpcNewClientWithNoProxy(t *testing.T) {


for the NoProxy and CustomDialer case, should we still override overrideHTTPSProxyFromEnvironment? That will make sure that even though proxy env is set we should skip it because of NoProxy and CustomDialer or is that not intended?

If WithNoProxy or WithContextDialer are set, the HTTPSProxyFromEnviornment function will never be called, but we can still override and add error if it is being called. Is it a good practice to do so? @purnesh42H

yeah so right now if HTTPSProxyFromEnviornment is not present, it doesn't matter if WithNoProxy or WithContextDialer is set? The resolution will happen at client anyways? So having it present and not being called in these two cases is what we want to verify.

purnesh42H · 2024-12-05T10:00:10Z

test/proxy_test.go

+		}
+		wantProxyAuthStr := "Basic " + base64.StdEncoding.EncodeToString([]byte(user+":"+password))
+		if got := req.Header.Get("Proxy-Authorization"); got != wantProxyAuthStr {
+			gotDecoded, _ := base64.StdEncoding.DecodeString(got)


we should not ignore decoding errors. it should return error if decoding fails

internal/transport/proxy.go

clientconn.go

dialoptions.go

internal/transport/http2_client.go

internal/transport/testutils/testutils.go

resolver_wrapper.go

internal/testutils/proxy.go

dfawley · 2025-01-16T18:38:51Z

internal/testutils/proxy.go

+	}()
+	t.Logf("Started proxy at: %q", pLis.Addr())
+	t.Cleanup(p.stop)
+	p.Addr = fmt.Sprintf("localhost:%d", ParsePort(t, pLis.Addr().String()))


Also why can't this just be pLis.Addr().String()? There should be a comment explaining why it's doing all this extra stuff to seemingly just convert localhost:123 into localhost:123.

pLis..Addr() will be a IP address, we convert it to localhost:port to make sure the DNS resolver is working correctly for the proxy and connecting to the proxy server.

dfawley · 2025-01-16T18:56:22Z

internal/transport/proxy.go

-		u := t.Username()
-		p, _ := t.Password()
+
+	if user := opts.User; user != (url.Userinfo{}) {


This comparison with a zero-initialized Userinfo looks a little off to me. Should this field in opts be a pointer instead to determine whether it was set intentionally?

Or should it check if either Username() or Password() are non-empty?

I'm not sure exactly what the right behavior is, but this seems questionable, as it's including any unexported fields in the comparison.

Related but potential existing problem to investigate: what if username is set but password is not? Is that valid? If so, is the proper behavior to encode it as username:, as it does currently, or should the : be omitted?

The RFC doesn't clearly mention what is the correct syntax if password is absent , so I think either way should be fine, but adding a semicolon like username: would be more explicit in indication username has ended and the password is empty.

I have added a UserSet option as changing it to a pointer can allow the caller to modify the use info.

eshitachandwani added the Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. label Nov 29, 2024

eshitachandwani added this to the 1.69 Release milestone Nov 29, 2024

eshitachandwani added the Type: Bug label Nov 29, 2024

eshitachandwani requested a review from purnesh42H November 30, 2024 10:44

eshitachandwani assigned purnesh42H Nov 30, 2024

Change proxy behaviour

619dcd4

eshitachandwani force-pushed the proxy_pr2 branch from 7fa029c to 619dcd4 Compare December 3, 2024 04:20

purnesh42H reviewed Dec 3, 2024

View reviewed changes

test/proxy_test.go Outdated Show resolved Hide resolved

test/proxy_test.go Outdated Show resolved Hide resolved

test/proxy_test.go Outdated Show resolved Hide resolved

eshitachandwani added 3 commits December 5, 2024 00:07

change delegating resolver

65b33bd

Improving code

94364e2

correct import

9f6a067

eshitachandwani requested a review from purnesh42H December 5, 2024 06:15

improve tests

5ce86d0

arjan-bal self-requested a review December 5, 2024 07:31

arjan-bal self-assigned this Dec 5, 2024

purnesh42H modified the milestones: 1.69 Release, 1.70 Release Dec 5, 2024

purnesh42H reviewed Dec 5, 2024

View reviewed changes

purnesh42H assigned eshitachandwani and unassigned purnesh42H Dec 5, 2024

eshitachandwani added 6 commits December 5, 2024 17:05

address comments

fbef3a4

add warning and httpfunc test

342c332

add warning and httpfunc test

8726188

from delegating_pr

1cfb089

improve code

32d9de5

improve code

34b902f

eshitachandwani requested a review from purnesh42H December 8, 2024 11:55

eshitachandwani assigned purnesh42H Dec 8, 2024

arjan-bal removed their assignment Jan 10, 2025

arjan-bal changed the title *: Correct gRFC A1 implementation *: Integrate delegating resolver and introduce dial options for target host resolution Jan 10, 2025

arjan-bal changed the title *: Integrate delegating resolver and introduce dial options for target host resolution transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution Jan 10, 2025

eshitachandwani requested a review from easwars January 10, 2025 10:34

comment for httpproxy

27cc055

dfawley assigned dfawley and unassigned easwars Jan 10, 2025

dfawley requested review from dfawley and removed request for easwars January 10, 2025 18:23

dfawley reviewed Jan 10, 2025

View reviewed changes

dfawley removed their assignment Jan 10, 2025

eshitachandwani added 8 commits January 14, 2025 16:08

address comments

227c748

remove helper

bdbe501

proxy helper function

8a43a91

proxy helper function

a73d511

proxy helper function

0c154cf

change proxy_utils

d396d31

change proxy_utils

fd088d1

change proxy_utils

32fa83c

eshitachandwani requested a review from dfawley January 16, 2025 07:32

eshitachandwani assigned dfawley and unassigned eshitachandwani Jan 16, 2025

dfawley reviewed Jan 16, 2025

View reviewed changes

dfawley assigned eshitachandwani and unassigned dfawley Jan 16, 2025

eshitachandwani added 2 commits January 17, 2025 03:30

addr comments

382bcb1

add userSet option

69037b0

eshitachandwani requested a review from dfawley January 17, 2025 08:52

eshitachandwani assigned dfawley and unassigned eshitachandwani Jan 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

eshitachandwani commented Nov 29, 2024 •

edited by arjan-bal

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

purnesh42H Dec 3, 2024

purnesh42H Dec 3, 2024

eshitachandwani Dec 4, 2024

purnesh42H Dec 3, 2024

eshitachandwani Dec 4, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

dfawley Jan 16, 2025

eshitachandwani Jan 16, 2025

dfawley Jan 16, 2025

eshitachandwani Jan 16, 2025

eshitachandwani Jan 17, 2025

		address := addr.Addr

		//if the ProxyConnectAddr is set in the aattribute, do a proxy dial.

transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

Are you sure you want to change the base?

transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

Conversation

eshitachandwani commented Nov 29, 2024 • edited by arjan-bal Loading

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eshitachandwani commented Nov 29, 2024 •

edited by arjan-bal

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading